-
Notifications
You must be signed in to change notification settings - Fork 56
Add ssqosid extension and csr yaml #653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ssqosid extension and csr yaml #653
Conversation
Signed-off-by: Syed Owais Ali Shah <113125582+syedowaisalishah@users.noreply.github.com>
Signed-off-by: Syed Owais Ali Shah <113125582+syedowaisalishah@users.noreply.github.com>
…ishah/riscv-unified-db into add-ssqosid-csr-yaml
@ThinkOpenly, I tested this using all files from PR #592. The first issue I noticed was a typo in mstateen0.yaml Also, I'm getting a type error on RV32:
But SRMCFG is defined in mstateen0.yaml like this:
Should I add something to explicitly include it for RV32? Or is there a better way to handle this? |
Are you on top of the latest commits for PR #592? The only references to "Ssqosid" are commented out.
It could be interesting to read the most recent discussion in #592. I think what we're going to need to define the "upper half" fields of those "state enable" CSRs in the "upper half" versions of those registers. Currently, they're defined as aliases, but that's not working at the moment. Regardless, for this situation, you'll need two cases, something like:
...but I confess I'm not sure that'l work because the code it still referencing the field from |
This is what I see in PR #592's latest version of
Is the typo in there? I'm not seeing it. |
Yes, there is a typo in the commented-out line:
It should be:
Just a small transposition of letters. Everything else looks good. |
Ah! I see it now. Amazing how that hides to my eyes. Could you add a review comment in #592 for that? |
It should be fixed now, sorry about that! |
So, ignore all that. There's much more discussion in #592 and the other more distant issues referenced there. The current understanding is that #592 will change to make that field defined for RV32, and the code emitting the type error will become correct. So, stand by. |
I believe #592 has been updated with the changes you needed here. Are you able to rebase on that now? |
@ThinkOpenly,Yes, I’ll check today after pulling the latest changes from the #592 merge and running the tests. I’ll update accordingly. |
@ThinkOpenly I got this error in sw_write():
Context code:
How should I define rcid_mask properly in IDL? |
There are many examples of declaring variables in other CSR files. In this particular case, maybe you could just avoid using a variable:
Refer to other implementations. Search for "WIDTH" in |
@ThinkOpenly ,Changes applied as suggested:
also added RCID_WIDTH and MCID_WIDTH as parameters in the extension YAML |
Hi @dhower-qc could you please take a look at this PR and approve it if everything looks good? It has one pending review request blocking the merge, and all checks have passed. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
This PR adds:
Ssqosid
Ssqosid
:srmcfg
Closes #567